Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass a function that doesn't return anything to FileUpload component #658

Merged
merged 1 commit into from
Nov 5, 2020
Merged

Pass a function that doesn't return anything to FileUpload component #658

merged 1 commit into from
Nov 5, 2020

Conversation

rubenvp8510
Copy link
Collaborator

@rubenvp8510 rubenvp8510 commented Nov 5, 2020 •
edited
Loading

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

Which problem is this PR solving?

Short description of the changes

  • In this PR: https://github.com/jaegertracing/jaeger-ui/pull/622/files a Redux middleware was updated to return the value of next(..) in order to preserve the Promises chain, which I think is correct.

  • The problem is that Upload.Dragger and our component FileUploader expect a function that returns nothing (...)=>void and the Upload component which is used by Upload.Dragger expect a function that returns nothing or a custom request, not a Promise, returning a Promise breaks the underling Upload component. Upload.Dragger (and our component FileUploader) has type definition for their parameters and Typescript is suppose to do some type checks here , but the search page was not migrated to TS.

Wondering if may be we should do the migration and enforce the types more to prevent future errors like this one. May be in a subsequent PR.

@yurishkuro
Copy link
Member

Thanks for the fix!

Wondering if may be we should do the migration and enforce the types more to prevent future errors like this one. May be in a subsequent PR.

Finishing the migration would be great, but difficult to staff. Could there be a unit test to catch this issue?

@yurishkuro
Copy link
Member

there is a linter error in CI

[prettier-lint] $ prettier --list-different '{.,scripts}/*.{js,json,md,ts,tsx}' 'packages/*/{src,demo/src}/**/!(layout.worker.bundled|react-vis).{css,js,json,md,ts,tsx}' 'packages/*/*.{css,js,json,md,ts,tsx}'
[eslint       ] $ eslint --cache 'scripts/*.{js,ts,tsx}' 'packages/*/src/**/*.{js,ts,tsx}' 'packages/*/*.{js,ts,tsx}'
[prettier-lint] packages/jaeger-ui/src/components/SearchTracePage/index.js
[prettier-lint] error Command failed with exit code 1.
[prettier-lint] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "prettier-lint" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The command "yarn lint" exited with 1.

@rubenvp8510
Pass a function that doesn't return anything to FileUpload component
86e7b54 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #658 into master will increase coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #658      +/-   ##
==========================================
+ Coverage   94.15%   94.20%   +0.05%     
==========================================
  Files         227      228       +1     
  Lines        5906     5923      +17     
  Branches     1484     1489       +5     
==========================================
+ Hits         5561     5580      +19     
+ Misses        305      304       -1     
+ Partials       40       39       -1
Impacted Files Coverage Δ
.../jaeger-ui/src/components/SearchTracePage/index.js 86.66% <0.00%> (-1.47%) ⬇️
...ackages/jaeger-ui/src/constants/default-config.tsx 100.00% <0.00%> (ø)
...es/jaeger-ui/src/components/DependencyGraph/DAG.js 100.00% <0.00%> (ø)
...src/components/TracePage/TraceGraph/TraceGraph.tsx 95.00% <0.00%> (ø)
...ckages/jaeger-ui/src/utils/version/get-version.tsx 100.00% <0.00%> (ø)
packages/jaeger-ui/src/components/App/TopNav.tsx 92.59% <0.00%> (+0.28%) ⬆️
...s/jaeger-ui/src/components/common/ErrorMessage.tsx 100.00% <0.00%> (+9.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00abad4...86e7b54. Read the comment docs.

@rubenvp8510
Copy link
Collaborator Author

Thanks for the fix!

Wondering if may be we should do the migration and enforce the types more to prevent future errors like this one. May be in a subsequent PR.

Finishing the migration would be great, but difficult to staff. Could there be a unit test to catch this issue?

Agree, it is difficult at this point, I can do a unit test for this case, my concern is if there is any other places where this could be a potential problem. Well, for now I think we are fine with a unit test here.

@yurishkuro
Copy link
Member

Well, for now I think we are fine with a unit test here.

yeah, but it didn't catch the issue with return value.

@yurishkuro yurishkuro merged commit b07aab8 into jaegertracing:master Nov 5, 2020
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
@rubenvp8510 @vvvprabhakar
Pass a function that doesn't return anything to FileUpload component (j…
a746138 
…aegertracing#658)

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
@rubenvp8510 @vvvprabhakar
Pass a function that doesn't return anything to FileUpload component (j…
c335dc2 
…aegertracing#658)

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
@rubenvp8510
Pass a function that doesn't return anything to FileUpload component (j…
5e1ea95 
…aegertracing#658)

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@everett980 everett980 Awaiting requested review from everett980

@tiffon tiffon Awaiting requested review from tiffon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty screen when click on a trace loaded from JSON
2 participants
@rubenvp8510 @yurishkuro